Skip to content

Conversation

kangqiwang
Copy link

@kangqiwang kangqiwang requested a review from WillAyd as a code owner March 26, 2025 11:51
@kangqiwang
Copy link
Author

Sorry for not adding a test case for this. I'll set it up once I get home and can work on my PC—my current laptop is too slow to handle the full testing environment.

@kangqiwang
Copy link
Author

Also, I noticed the unit tests failed. Again, I will fix them once I get home.

@a-holm
Copy link

a-holm commented Mar 31, 2025

Thanks for working on this fix for #60647!

This change successfully removes the code path containing the specific line (holidays = holidays + calendar.holidays().tolist()) identified in the issue and the comments.

However, there are a few points that probably deserve some attention:

Removed Functionality: This fix works by removing the if-else block. Was the functionality within that block (handling a non-numpy calendar object passed with holidays) intended or ever valid? Removing it is a potentially breaking change that should ideally be documented.

Exception Type: Raising ApplyTypeError here is a bit unusual, as it's typically for arithmetic operations returning NotImplemented. Would a ValueError or TypeError be more appropriate to signal invalid arguments during the setup phase?

@mroeschke
Copy link
Member

Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.

@mroeschke mroeschke closed this Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

BUG: CustomBusinessDay not respecting calendar

3 participants